Add `FileScanConfigBuilder` #15352

blaginin · 2025-03-21T17:01:04Z

Rationale for this change

FileScanConfig now violates single responsibility from SOLID. It serves two conflicting roles:

As a builder, though this should be changed as discussed in

datafusion/datafusion/datasource/src/file_scan_config.rs

Line 631 in 635e73b

// TODO: This function should be moved into DataSourceExec once FileScanConfig moved out of datafusion/core
As a business logic provider (e.g., fn project, impl DataSource, etc.)

These conflicting roles lead to issues like #14905 and #14679, where provider features are accessed even before the build process is complete.

What changes are included in this PR?

I've added FileScanConfigBuilder and deprecated builder approach for FileScanConfig

Are these changes tested?

Yes, updated exiting tests into the new interface

Are there any user-facing changes?

Yes, new builder interface - but the switch is quite easy (all builder-methods from FileScanConfig are supported)

blaginin · 2025-03-21T17:07:22Z

datafusion-examples/examples/parquet_index.rs

-            FileScanConfig::new(object_store_url, self.schema(), source)
+            FileScanConfigBuilder::new(object_store_url, self.schema(), source)
                .with_projection(projection.cloned())
-                .with_limit(limit);
+                .with_limit(limit)
+                .build();


@mertak-synnada, hey, this PR is still WIP but I was wondering if you're happy with this approach. That's what we've discussed in #14685 (comment)

Looking good so far, thank you! I haven't been able to test it thoroughly yet, but the legacy ParquetExecBuilder could be helpful for understanding specific cases, just FYI.

blaginin · 2025-03-24T21:22:48Z

also fyi @AdamGS 👀

mertak-synnada · 2025-03-25T10:08:45Z

datafusion-examples/examples/advanced_parquet_index.rs


        // Finally, put it all together into a DataSourceExec
-        Ok(file_scan_config.build())
+        Ok(Arc::new(DataSourceExec::new(Arc::new(file_scan_config))))


Isn't it possible to have this return as a function? Is it because of import cycles?

you mean as aDataSourceExec function? It also looks a bit verbose to me, but the inner Arc is needed for dynamic dispatch, and the outer one makes the return type more explicit. Happy to make it DataSourceExec::new_arc if you want, but i don't think we use that a lot in datafusion

I meant just a cosmetic change to not re-writing the whole Arc::new(DataSourceExec::new(Arc::new(file_scan))) Maybe it can be something like DataSourceExec::from_file_source(file_scan) -> Arc<DataSourceExec>

What would you think?

do you like 748a335 ? Looks cleaner

AdamGS · 2025-03-25T18:38:40Z

my 2c - this looks great, I would love to also rename FileScanConfig to something else but I get the backwards compatibility concerns.

alamb

Thanks @blaginin -- this looks great to me. Thank you !

I think it is really nice that it is backwards compatible as well (deprecated old APIs rather than removing)

I have one request for some more documentation, but I think this one is really good now

alamb · 2025-03-26T21:13:38Z

datafusion/datasource/src/file_scan_config.rs

@@ -174,6 +175,219 @@ pub struct FileScanConfig {
    pub batch_size: Option<usize>,
 }

+#[derive(Clone)]
+pub struct FileScanConfigBuilder {


Can we please add comments and examples here? Perhaps just pointing back to FileScanConfig?

Added docs for methods and example

alamb · 2025-03-26T21:14:53Z

datafusion/datasource/src/file_scan_config.rs

@@ -326,14 +544,15 @@ impl FileScanConfig {
    /// # Parameters:
    /// * `object_store_url`: See [`Self::object_store_url`]
    /// * `file_schema`: See [`Self::file_schema`]
+    #[allow(deprecated)] // `new` will be removed same time as `with_source`


this is nice that it will deprecate the old API rather and will guide users during upgrade

mertak-synnada · 2025-03-27T08:13:56Z

datafusion-examples/examples/advanced_parquet_index.rs


        // Finally, put it all together into a DataSourceExec
-        Ok(file_scan_config.build())
+        Ok(Arc::new(DataSourceExec::new(Arc::new(file_scan_config))))


I meant just a cosmetic change to not re-writing the whole Arc::new(DataSourceExec::new(Arc::new(file_scan))) Maybe it can be something like DataSourceExec::from_file_source(file_scan) -> Arc<DataSourceExec>

What would you think?

mertak-synnada · 2025-03-27T08:17:31Z

datafusion/datasource/src/file_scan_config.rs

@@ -645,6 +875,7 @@ impl FileScanConfig {

    // TODO: This function should be moved into DataSourceExec once FileScanConfig moved out of datafusion/core


We may also remove this TODO with above suggestion I believe

mertak-synnada

Thanks! Looking good to me

alamb · 2025-03-28T18:00:10Z

Thanks again @blaginin @mertak-synnada

* WIP: Add `FileScanConfigBuilder` and switch some cases * Fmt * More fmt * Clean `FileScanConfig::build` * Clean `FileScanConfig::new` * Fix csv + fmt * More fixes * Remove pub * Remove todo * Add usage example * Fix input type * Add `from_data_source` * Fmt * Add docs for `with_source`

WIP: Add FileScanConfigBuilder and switch some cases

b7ae6ed

github-actions bot added core Core DataFusion crate proto Related to proto crate datasource Changes to the datasource crate labels Mar 21, 2025

blaginin added 2 commits March 21, 2025 17:05

Fmt

93002ea

More fmt

dc53de6

blaginin commented Mar 21, 2025

View reviewed changes

blaginin added 2 commits March 24, 2025 20:09

Merge main & add batch_size

c936329

Clean FileScanConfig::build

c42c906

github-actions bot added the substrait Changes to the substrait crate label Mar 24, 2025

blaginin changed the title ~~WIP: Add FileScanConfigBuilder and switch some cases~~ WIP: Add FileScanConfigBuilder Mar 24, 2025

blaginin added 3 commits March 24, 2025 21:04

Clean FileScanConfig::new

e13bfa9

Fix csv + fmt

3c216b8

More fixes

1fc4b3d

blaginin marked this pull request as ready for review March 24, 2025 21:22

blaginin changed the title ~~WIP: Add FileScanConfigBuilder~~ Add FileScanConfigBuilder Mar 24, 2025

blaginin mentioned this pull request Mar 24, 2025

Statistics::total_byte_size does not account for projection in FileScanConfig::with_projection #14936

Open

blaginin requested a review from mertak-synnada March 24, 2025 21:24

Remove pub

ae32a01

mertak-synnada reviewed Mar 25, 2025

View reviewed changes

blaginin requested a review from mertak-synnada March 26, 2025 14:25

Merge main + deprecate more methods

4501798

alamb approved these changes Mar 26, 2025

View reviewed changes

mertak-synnada reviewed Mar 27, 2025

View reviewed changes

blaginin added 4 commits March 27, 2025 13:42

Merge main

f0b33d9

Remove todo

7e3521c

Add usage example

d13dfc5

Fix input type

e1a056d

blaginin added 3 commits March 27, 2025 14:33

Add from_data_source

748a335

Fmt

958f81e

Add docs for with_source

45b6175

blaginin requested a review from mertak-synnada March 27, 2025 14:50

mertak-synnada approved these changes Mar 28, 2025

View reviewed changes

alamb merged commit 4a3f70c into apache:main Mar 28, 2025
27 checks passed

blaginin mentioned this pull request Mar 28, 2025

Remove redundant statistics from FileScanConfig #14955

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `FileScanConfigBuilder` #15352

Add `FileScanConfigBuilder` #15352

blaginin commented Mar 21, 2025 •

edited

Loading

blaginin Mar 21, 2025

mertak-synnada Mar 24, 2025

blaginin commented Mar 24, 2025

mertak-synnada Mar 25, 2025

blaginin Mar 25, 2025

mertak-synnada Mar 27, 2025

blaginin Mar 27, 2025

AdamGS commented Mar 25, 2025

alamb left a comment

alamb Mar 26, 2025

blaginin Mar 27, 2025

alamb Mar 26, 2025

mertak-synnada Mar 27, 2025

mertak-synnada Mar 27, 2025

blaginin Mar 27, 2025

mertak-synnada left a comment

alamb commented Mar 28, 2025

		@@ -645,6 +875,7 @@ impl FileScanConfig {

		// TODO: This function should be moved into DataSourceExec once FileScanConfig moved out of datafusion/core

Add FileScanConfigBuilder #15352

Add FileScanConfigBuilder #15352

Conversation

blaginin commented Mar 21, 2025 • edited Loading

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blaginin commented Mar 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdamGS commented Mar 25, 2025

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mertak-synnada left a comment

Choose a reason for hiding this comment

alamb commented Mar 28, 2025

Add `FileScanConfigBuilder` #15352

Add `FileScanConfigBuilder` #15352

blaginin commented Mar 21, 2025 •

edited

Loading